Skip to content

Enable Wemo option for subscriptions across subnets#75412

Closed
jherby2k wants to merge 1 commit into
home-assistant:devfrom
jherby2k:wemo-callback-address
Closed

Enable Wemo option for subscriptions across subnets#75412
jherby2k wants to merge 1 commit into
home-assistant:devfrom
jherby2k:wemo-callback-address

Conversation

@jherby2k
Copy link
Copy Markdown

@jherby2k jherby2k commented Jul 18, 2022 •
edited
Loading

Proposed change

Enables setting a "callback_address" (advanced option) in the Wemo integration to enable subscriptions when HA and the Wemo switches are on different subnets. Eliminates a recurring error message, and allows for instantanous updates when a Wemo device is operated physically or through the Wemo mobile app.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Deprecation (breaking change to happen in the future)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

PYWEMO_CALLBACK_ADDRESS was recently enabled in pywemo to address this issue. This PR simply exposes a way to set this environment variable from HA.

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

To help with the load of incoming pull requests:

@probot-home-assistant
Copy link
Copy Markdown

probot-home-assistant Bot commented Jul 18, 2022

Hey there @esev, mind taking a look at this pull request as it has been labeled with an integration (wemo) you are listed as a code owner for? Thanks!
(message by CodeOwnersMention)

@jherby2k jherby2k changed the title Update __init__.py Enable Wemo option for working across VLANs Jul 18, 2022
@jherby2k jherby2k changed the title Enable Wemo option for working across VLANs Enable Wemo option for subscriptions across VLANs Jul 18, 2022
esev
esev approved these changes Jul 18, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@esev esev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

frenck
frenck requested changes Jul 18, 2022
View reviewed changes
Comment thread homeassistant/components/wemo/__init__.py
{
DOMAIN: vol.Schema(
{
vol.Optional(CONF_CALLBACK_ADDRESS): cv.string,
Copy link
Copy Markdown
Member

@frenck frenck Jul 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be provided by the Network integration instead?

Additionally:

For integrations that connect to devices or services, we no longer accept new YAML configuration or changes.

This integration needs to be refactored first to use a configuration flow and config entries first.

More information about this can be found in Architecture Decision Record:

See our developer documentation on how to get started creating a configuration flow for this integration:

https://developers.home-assistant.io/docs/config_entries_config_flow_handler

As these changes often involve a bit of work and some significant shift in the current code, we will close this PR for now.

We (and our community!) would really appreciate it if you could start on the refactoring of this integration in a new PR.

Thanks already! 👍

Copy link
Copy Markdown
Author

@jherby2k jherby2k Jul 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see how this would go in the Network integration... it configures a pywemo-specific environment variable. Anyway, will rework after config flow is done. Beyond my current capabilities.

@frenck frenck closed this Jul 18, 2022
@esev
Copy link
Copy Markdown
Contributor

esev commented Jul 18, 2022 •
edited
Loading

Hi @frenck, could you provide feedback on #56972? I know I need to fix the merge conflict, but is the approach okay?

I've previously been told that integrations should not use a config entry per device. So that's why I went the route I did for #56972, making it a global config for the integration.

TBH, I have no idea how to replicate the static config without having a config entry per device.

@frenck
Copy link
Copy Markdown
Member

frenck commented Jul 18, 2022 •
edited
Loading

Hi @frenck, could you provide feedback on #56972? I know I need to fix the merge conflict, but is the approach okay?

That is not related to this PR? I'm confused?

I've #44192 (comment) that integrations should not use a config entry per device.

Depends on who you ask and the use case. Some integrations benefit from using a per device approach. A good example is LIFX for which is a PR open right now to migrate to a per device based approach because of similar issues I guess 🤷

@esev
Copy link
Copy Markdown
Contributor

esev commented Jul 18, 2022 •
edited
Loading

Thank you! It's not directly related, but if the approach is okay I think this configuration option could be put in the same spot.

Edit: frenck said it should go in the Network integration instead. So definitely not related.

@jherby2k jherby2k mentioned this pull request Jul 18, 2022
Closed
8 tasks
@jherby2k jherby2k changed the title Enable Wemo option for subscriptions across VLANs Enable Wemo option for subscriptions across subnets Jul 18, 2022
@github-actions github-actions Bot locked and limited conversation to collaborators Jul 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@frenck frenck frenck requested changes

+1 more reviewer

@esev esev esev approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

bugfix cla-signed integration: wemo small-pr PRs with less than 30 lines.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jherby2k @esev @frenck @homeassistant